Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move out tendermint::config to tendermint-config crate #986

Merged
merged 19 commits into from
Oct 7, 2021

Conversation

soareschen
Copy link
Contributor

@soareschen soareschen commented Sep 20, 2021

Fixes #983 and #985.

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Added entry in .changelog/

@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2021

Codecov Report

Merging #986 (fde78cd) into master (e3e4c88) will decrease coverage by 0.0%.
The diff coverage is 94.3%.

❗ Current head fde78cd differs from pull request most recent head 097b434. Consider uploading reports for the commit 097b434 to get more accurate results
Impacted file tree graph

@@           Coverage Diff            @@
##           master    #986     +/-   ##
========================================
- Coverage    72.7%   72.7%   -0.1%     
========================================
  Files         204     206      +2     
  Lines       16631   16641     +10     
========================================
+ Hits        12101   12107      +6     
- Misses       4530    4534      +4     
Impacted Files Coverage Δ
config/src/config.rs 63.4% <ø> (ø)
config/src/error.rs 0.0% <0.0%> (ø)
config/src/node_key.rs 56.5% <0.0%> (ø)
rpc/src/client/transport/http.rs 0.0% <ø> (ø)
rpc/src/client/transport/websocket.rs 63.7% <ø> (-0.2%) ⬇️
tendermint/src/error.rs 0.0% <ø> (ø)
tendermint/src/lib.rs 100.0% <ø> (ø)
config/src/net.rs 92.3% <83.3%> (ø)
config/tests/mod.rs 96.1% <96.1%> (ø)
config/src/lib.rs 100.0% <100.0%> (ø)
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3e4c88...097b434. Read the comment docs.

Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The metadata in the new cargo toml should be catered to the actual crate instead of a verbatim copy of the tendermint cargo manifest.

@soareschen
Copy link
Contributor Author

The metadata in the new cargo toml should be catered to the actual crate instead of a verbatim copy of the tendermint cargo manifest.

Yeah I missed updating that after copying the boilerplate. Fixed it now.

@soareschen
Copy link
Contributor Author

Rehashing my note in #983:

I have taken a look at the use of net::Address, and it does not seem like the actual details are used anywhere. In #986 I moved out tendermint::net to tendermint_config::net, and changed the field in tendermint::node::OtherInfo::rpc_address to just String. If anyone needs to do the actual URL parsing, they can do it at the call site instead.

The main difference I see of using String instead of Address for rpc_address is the way it is deserialized. With the Deserialize implementation provided by Address, an invalid URL would result in an error. However I also do not see how this can be solved using feature flag. In the absense of the url crate, should the deserialization just succeed? That wouldn't be good since the crate would effectively be behaving differently with no invariant in place. Otherwise if it is possible to implement the parsing without using url, then it is better off to just not url in both cases in the first place.

Note that the other alternative is to just wait for servo/rust-url#717 to get merged and released.

config/src/net.rs Show resolved Hide resolved
light-client/src/components/verifier.rs Outdated Show resolved Hide resolved
light-client/src/components/verifier.rs Outdated Show resolved Hide resolved
tendermint/src/lib.rs Outdated Show resolved Hide resolved
config/src/priv_validator_key.rs Show resolved Hide resolved
config/Cargo.toml Outdated Show resolved Hide resolved
config/Cargo.toml Outdated Show resolved Hide resolved
rpc/Cargo.toml Outdated Show resolved Hide resolved
config/tests/mod.rs Outdated Show resolved Hide resolved
abci/src/lib.rs Outdated Show resolved Hide resolved
@@ -82,7 +76,7 @@ pub struct OtherInfo {
pub tx_index: TxIndexStatus,

/// RPC address
pub rpc_address: net::Address,
pub rpc_address: String,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One unresolved issue is whether using String instead of net::Address for rpc_address will break any functionality. The main difference is in the implementations of Deserialize and Display. Any idea of how this field is used in practice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thanethomson Might have some input here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea here actually.

thanethomson and others added 4 commits October 6, 2021 09:32
* Fix `cargo test` and add check to CI (#990)

* Relocate ABCI test to fix broken doctest

Signed-off-by: Thane Thomson <[email protected]>

* Use tokio_test for mock client doctest

Signed-off-by: Thane Thomson <[email protected]>

* Add CI test for default features

Signed-off-by: Thane Thomson <[email protected]>

* Add `block_search` RPC endpoint (#991)

* Add block_search RPC endpoint and tests

* Add .changelog entry

* Fix comments

* tools: Fix `block_search` endpoint integration tests (#999)

Closes #998

* Bump integration test tendermint to v0.34.13
* Fix kvstore integration tests
* Bump tendermint version to v0.34.13 in CI

Signed-off-by: Thane Thomson <[email protected]>

* ci: Build and check tools (#997)

So far only the kvstore tests ran as part of the Github workfows. This
would leave opportunity for changes to introduce breakage to the builds
of the tools. In this change the same build and clippy stages are
introduced for the tools workspace that currently run for the top-level
one.

Signed-off-by: xla <[email protected]>

* tools: Add `block_search` method to RPC probe (#1002)

* Add missing block_search endpoint
* Bump tendermint version to v0.34.13

Signed-off-by: Thane Thomson <[email protected]>

Co-authored-by: Shoaib Ahmed <[email protected]>
Co-authored-by: xla <[email protected]>
Signed-off-by: Thane Thomson <[email protected]>
@soareschen soareschen added the no_std no_std compatibility label Oct 7, 2021
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say let's go ahead and merge this. Thanks @soareschen! 🎉

@thanethomson thanethomson merged commit 1ab5e86 into master Oct 7, 2021
@thanethomson thanethomson deleted the soares/move-tendermint-config branch October 7, 2021 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no_std no_std compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move config module out from tendermint into its own crate
5 participants